Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

Fix - Bank::compute_active_feature_set() and Bank::apply_feature_activations() #34124

Merged
merged 3 commits into from
Nov 17, 2023
Merged

Fix - Bank::compute_active_feature_set() and Bank::apply_feature_activations() #34124

merged 3 commits into from
Nov 17, 2023

Conversation

Lichtso
Copy link
Contributor

@Lichtso Lichtso commented Nov 16, 2023

Problem

A feature activation on devnet diverged because it was activated before the epoch boundary on part of the cluster (version dependent). This happened because of the backport #34003 which depends on Bank::compute_active_feature_set() being without side effects (as the name suggests).

Summary of Changes

Move the actual activation of pending features from Bank::compute_active_feature_set() into Bank::apply_feature_activations().

@Lichtso
Moves modification of feature accounts from Bank::compute_active_feat…
a730a72 
…ure_set() into Bank::apply_feature_activations().
@Lichtso Lichtso added the v1.17 PRs that should be backported to v1.17 label Nov 16, 2023
@steviez
Copy link
Contributor

steviez commented Nov 16, 2023 •
edited
Loading

I'm inclined to think this unit test will now fail:

solana/runtime/src/bank/tests.rs

Line 7727 in e589c07

fn test_compute_active_feature_set() {

Aside from Bank::apply_feature_activations(), this unit test looks to be the only other caller of Bank::compute_active_feature_set() so that unit test should be the only other things that needs to be updated

@pgarg66
Copy link
Contributor

pgarg66 commented Nov 16, 2023

I'm inclined to think this unit test will now fail:

solana/runtime/src/bank/tests.rs

Line 7727 in e589c07

fn test_compute_active_feature_set() {

Aside from Bank::apply_feature_activations(), this unit test looks to be the only other caller of Bank::compute_active_feature_set() so that unit test should be the only other things that needs to be updated

You're right. That test is failing.

@steviez steviez mentioned this pull request Nov 16, 2023
Merged
@codecov Codecov
Copy link

codecov bot commented Nov 16, 2023

Codecov Report

Merging #34124 (0942241) into master (b4c652e) will decrease coverage by 0.1%.
Report is 8 commits behind head on master.
The diff coverage is 100.0%.

Additional details and impacted files 
@@            Coverage Diff            @@
##           master   #34124     +/-   ##
=========================================
- Coverage    81.9%    81.9%   -0.1%     
=========================================
  Files         816      816             
  Lines      219753   219759      +6     
=========================================
+ Hits       180001   180004      +3     
- Misses      39752    39755      +3

@Lichtso Lichtso requested a review from steviez November 17, 2023 00:24
@steviez steviez requested a review from AshwinSekar November 17, 2023 00:51
steviez
steviez approved these changes Nov 17, 2023
View reviewed changes
Copy link
Contributor

@steviez steviez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@Lichtso Lichtso merged commit 6b85450 into master Nov 17, 2023
17 checks passed
@Lichtso Lichtso deleted the fix/compute_active_feature_set branch November 17, 2023 08:54
mergify bot pushed a commit that referenced this pull request Nov 17, 2023
@Lichtso @mergify
Fix - Bank::compute_active_feature_set() and `Bank::apply_feature_a…
3157d63 
…ctivations()` (#34124)

* Moves modification of feature accounts from Bank::compute_active_feature_set() into Bank::apply_feature_activations().

* Renames allow_new_activations and newly_activated to include_pending and pending.

* Fix test_compute_active_feature_set.

(cherry picked from commit 6b85450)
@mergify mergify bot mentioned this pull request Nov 17, 2023
Merged
Lichtso added a commit that referenced this pull request Nov 17, 2023
@mergify @Lichtso
v1.17: Fix - Bank::compute_active_feature_set() and `Bank::apply_fe…
cf1d9d4 
…ature_activations()` (backport of #34124) (#34136)

Fix - `Bank::compute_active_feature_set()` and `Bank::apply_feature_activations()` (#34124)

* Moves modification of feature accounts from Bank::compute_active_feature_set() into Bank::apply_feature_activations().

* Renames allow_new_activations and newly_activated to include_pending and pending.

* Fix test_compute_active_feature_set.

(cherry picked from commit 6b85450)

Co-authored-by: Alexander Meißner <[email protected]>
@HaoranYi HaoranYi mentioned this pull request Apr 8, 2024
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@AshwinSekar AshwinSekar AshwinSekar approved these changes

@steviez steviez steviez approved these changes

Assignees
No one assigned
Labels
v1.17 PRs that should be backported to v1.17
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Lichtso @steviez @pgarg66 @AshwinSekar